Skip to content

fix docs navigation stability and search filter interactions#772

Merged
tannerlinsley merged 1 commit intomainfrom
site-reliability
Mar 17, 2026
Merged

fix docs navigation stability and search filter interactions#772
tannerlinsley merged 1 commit intomainfrom
site-reliability

Conversation

@LadyBluenotes
Copy link
Member

@LadyBluenotes LadyBluenotes commented Mar 17, 2026

Summary

  • fix docs sidebar group toggling so collapsible sections behave reliably in Chrome
  • fix search palette filter dropdown interactions so selectors can reopen and switch correctly inside the modal
  • handle invalid library route params with proper notFound() behavior instead of throwing runtime errors
  • make the smoke test dev-server startup more resilient and easier to debug during pre-commit
  • clean up lint noise from known TanStack Table React Compiler warnings in admin routes

Issues addressed

Details

Docs navigation

The docs sidebar was relying on render-time <details open> behavior instead of explicit React state, which could break collapsible group interaction in Chrome. This change makes group open/close state controlled and keeps the active section expanded.

Sidebar link preloading is also disabled in this path to reduce the chance of hover-triggered instability on heavy docs pages.

Search modal

The search palette library/framework selectors were using dropdown trigger composition that could interfere with focus and reopening behavior inside the dialog. This change switches them to proper button triggers and uses non-modal dropdown behavior inside the modal.

Invalid route handling

Several /$libraryId/... routes now use findLibrary(...) with notFound() so invalid library IDs render a proper not-found state instead of throwing.

Tooling stability

The smoke test now allows more time for the dev server to start while tsc and eslint run in parallel, and it prints recent server output if startup times out.

Admin route useReactTable(...) call sites now explicitly suppress the React Compiler incompatible-library warning, which keeps lint output clean without changing table behavior.

Validation

  • pnpm run lint
  • pnpm test

Summary by CodeRabbit

  • Bug Fixes

    • Improved error handling for missing libraries and versions with proper not-found responses.
  • Improvements

    • Enhanced dropdown and button component interactions in search refinements.
    • Refined menu group state management for improved navigation and collapsible section behavior.

@netlify
Copy link

netlify bot commented Mar 17, 2026

Deploy Preview for tanstack ready!

Name Link
🔨 Latest commit decb771
🔍 Latest deploy log https://app.netlify.com/projects/tanstack/deploys/69b9a83184759b000878eae1
😎 Deploy Preview https://deploy-preview-772--tanstack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 30 (🔴 down 4 from production)
Accessibility: 90 (no change from production)
Best Practices: 83 (🔴 down 9 from production)
SEO: 97 (no change from production)
PWA: 70 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai
Copy link

coderabbitai bot commented Mar 17, 2026

📝 Walkthrough

Walkthrough

This PR addresses multiple bugs and improvements: fixes Chrome sidebar navigation through menu state management refactoring in DocsLayout, resolves dropdown reopening issues in the search modal by removing modal context blocking, adds proper error handling for missing libraries via notFound() guards across route files, and adds ESLint suppression comments for react-hooks compatibility in admin routes.

Changes

Cohort / File(s) Summary
Menu and Navigation State Management
src/components/DocsLayout.tsx
Refactored menu group state management from dynamic rendering to unified groupKey-based mechanism with memoized open state initialization, conditional details/div rendering, and onToggle event handling for group toggling.
Search Modal Dropdown Fixes
src/components/SearchModal.tsx
Changed Dropdown components from modal={true} to modal={false} in refinement components, replaced DropdownTrigger asChild={false} with standard button element, and added explicit type="button" for correct semantics.
Library Not Found Error Handling
src/routes/$libraryId/$version.docs.tsx, src/routes/$libraryId/$version.index.tsx, src/routes/$libraryId/$version.tsx
Replaced getLibrary with findLibrary across route head and component paths, added notFound() guards to handle missing libraries, and added version validation logic to prevent proceeding with undefined library data.
Admin Routes ESLint Suppression
src/routes/admin/audit.tsx, src/routes/admin/github-stats.tsx, src/routes/admin/logins.tsx, src/routes/admin/npm-stats.tsx, src/routes/admin/roles.$roleId.tsx
Added ESLint disable-next-line directives for react-hooks/incompatible-library rule on useReactTable hook invocations across multiple admin pages.
Import Cleanup
src/components/CopyPageDropdown.tsx
Removed unused PACKAGE_MANAGERS import while retaining getPackageManager function import.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 Menus now toggle with grace in Chrome's embrace,
Dropdowns reopen, no more the dreaded space,
Libraries found (or not), handled with care,
With notFound guards keeping errors at bay everywhere! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title clearly summarizes the main changes: fixing docs navigation stability and search filter interactions, which aligns with the primary objectives of addressing sidebar issues and dropdown behavior.
Linked Issues check ✅ Passed All primary requirements from linked issues are addressed: DocsLayout changes restore sidebar stability [#688], SearchModal changes fix dropdown reopening [#650, #753], route param handling uses findLibrary with notFound [#504], and link preloading disabled reduces instability [#718].
Out of Scope Changes check ✅ Passed All code changes align with stated objectives: DocsLayout refactoring and link preloading for navigation stability, SearchModal refinements for filter interactions, library route error handling, and ESLint suppressions for admin page warnings.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch site-reliability
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/routes/admin/npm-stats.tsx (1)

271-290: ⚠️ Potential issue | 🟡 Minor

Missing ESLint suppression on second useReactTable call.

The suppression comment is added for libraryTable at line 272, but the packagesTable at line 286 also uses useReactTable and lacks the same suppression. If the react-hooks/incompatible-library rule triggers on useReactTable calls, both instances should be suppressed for consistency.

🔧 Proposed fix
   // eslint-disable-next-line react-hooks/incompatible-library
   const libraryTable = useReactTable({
     data: libraryStats ?? [],
     columns: libraryColumns,
     getCoreRowModel: getCoreRowModel(),
   })
 
   const packages = useMemo(
     () =>
       [...(packagesData?.packages ?? [])].sort(
         (a, b) => (b.downloads ?? 0) - (a.downloads ?? 0),
       ),
     [packagesData?.packages],
   )
 
+  // eslint-disable-next-line react-hooks/incompatible-library
   const packagesTable = useReactTable({
     data: packages,
     columns: packageColumns,
     getCoreRowModel: getCoreRowModel(),
   })
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/routes/admin/npm-stats.tsx` around lines 271 - 290, The second use of
useReactTable (creating packagesTable) is missing the ESLint suppression present
for libraryTable; add the same disable comment (eslint-disable-next-line
react-hooks/incompatible-library) immediately above the packagesTable
declaration so both calls to useReactTable (libraryTable and packagesTable)
consistently suppress the react-hooks/incompatible-library rule.
🧹 Nitpick comments (3)
src/components/DocsLayout.tsx (2)

669-674: Non-collapsible wrapper has unnecessary summary-targeting CSS classes.

The [&>summary]:before:mr-1 [&>summary]:marker:text-[0.8em] [&>summary]:marker:leading-4 classes only apply to a <summary> child, but non-collapsible groups render a <div> child inside this wrapper. These selectors will never match.

Consider extracting shared classes or removing the summary-specific selectors from the non-collapsible case:

♻️ Proposed fix
     ) : (
       <div
         key={`group-${i}`}
-        className="[&>summary]:before:mr-1 [&>summary]:marker:text-[0.8em] [&>summary]:marker:leading-4 relative select-none"
+        className="relative select-none"
       >
         {groupContent}
       </div>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/components/DocsLayout.tsx` around lines 669 - 674, The wrapper div
rendering non-collapsible groups contains summary-specific CSS selectors
("[&>summary]:before:mr-1 [&>summary]:marker:text-[0.8em]
[&>summary]:marker:leading-4") that will never match because it renders a <div>
child (see the wrapper with key={`group-${i}`} and variable groupContent);
remove those summary-targeting classes from this non-collapsible wrapper or
extract the shared, non-summary classes into a common variable and apply only
summary-specific selectors on the collapsible wrapper/element that actually
contains a <summary>, so update the className on the non-collapsible wrapper to
omit the [&>summary] rules and keep shared styles centralized.

537-550: Consider using a more stable key than String(group.label) for ReactNode labels.

When group.label is a React element (e.g., the GitHub/YouTube/Discord menu items), String(group.label) returns "[object Object]". The index prefix prevents collisions, but if menu items are conditionally included/excluded, the same group could get different keys across renders, causing unexpected open/close state changes.

Consider adding a stable id field to MenuItem type, or using a hash of the label string when available:

const key = typeof group.label === 'string' 
  ? `${index}:${group.label}` 
  : `${index}:group-${index}`
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/components/DocsLayout.tsx` around lines 537 - 550, The current key
generation in groupInitialOpenState uses String(group.label) which is unstable
for ReactNode labels; update the key logic to use a stable identifier: prefer an
explicit id property on the group (e.g., group.id) if present, otherwise use the
label when it's a string, and as a final fallback produce a deterministic
fallback based on the group's index (e.g., `group-${index}`); update the key
assignment in the reduce callback (the variable currently named key) and ensure
any type/interface for menuConfig/MenuItem supports an optional id so state
remains consistent across conditional renders.
src/routes/$libraryId/$version.tsx (1)

13-19: Consider centralizing the repeated findLibrary/notFound() guard.

The same lookup/404 block now lives in beforeLoad, loader, and RouteForm, with the same pattern repeated in the sibling routes in this PR. A small shared helper would make future changes to library resolution or 404 behavior much easier to keep consistent.

♻️ Helper sketch
+function requireLibrary(libraryId: string) {
+  const library = findLibrary(libraryId)
+  if (!library) {
+    throw notFound()
+  }
+  return library
+}
+
   beforeLoad: (ctx) => {
     const { libraryId, version } = ctx.params
-    const library = findLibrary(libraryId)
-
-    if (!library) {
-      throw notFound()
-    }
+    const library = requireLibrary(libraryId)

Also applies to: 31-35, 53-57

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/routes/`$libraryId/$version.tsx around lines 13 - 19, The repeated
pattern using findLibrary(...) followed by throw notFound() should be
centralized: add a small helper (e.g., ensureLibraryOrThrow(libraryId) or
resolveLibraryOr404) that calls findLibrary and throws notFound() when missing,
then replace the duplicate blocks in beforeLoad, loader, and the RouteForm
component with calls to that helper (use the helper name in each location to
make intent clear and keep behavior consistent across sibling routes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/routes/admin/npm-stats.tsx`:
- Around line 271-290: The second use of useReactTable (creating packagesTable)
is missing the ESLint suppression present for libraryTable; add the same disable
comment (eslint-disable-next-line react-hooks/incompatible-library) immediately
above the packagesTable declaration so both calls to useReactTable (libraryTable
and packagesTable) consistently suppress the react-hooks/incompatible-library
rule.

---

Nitpick comments:
In `@src/components/DocsLayout.tsx`:
- Around line 669-674: The wrapper div rendering non-collapsible groups contains
summary-specific CSS selectors ("[&>summary]:before:mr-1
[&>summary]:marker:text-[0.8em] [&>summary]:marker:leading-4") that will never
match because it renders a <div> child (see the wrapper with key={`group-${i}`}
and variable groupContent); remove those summary-targeting classes from this
non-collapsible wrapper or extract the shared, non-summary classes into a common
variable and apply only summary-specific selectors on the collapsible
wrapper/element that actually contains a <summary>, so update the className on
the non-collapsible wrapper to omit the [&>summary] rules and keep shared styles
centralized.
- Around line 537-550: The current key generation in groupInitialOpenState uses
String(group.label) which is unstable for ReactNode labels; update the key logic
to use a stable identifier: prefer an explicit id property on the group (e.g.,
group.id) if present, otherwise use the label when it's a string, and as a final
fallback produce a deterministic fallback based on the group's index (e.g.,
`group-${index}`); update the key assignment in the reduce callback (the
variable currently named key) and ensure any type/interface for
menuConfig/MenuItem supports an optional id so state remains consistent across
conditional renders.

In `@src/routes/`$libraryId/$version.tsx:
- Around line 13-19: The repeated pattern using findLibrary(...) followed by
throw notFound() should be centralized: add a small helper (e.g.,
ensureLibraryOrThrow(libraryId) or resolveLibraryOr404) that calls findLibrary
and throws notFound() when missing, then replace the duplicate blocks in
beforeLoad, loader, and the RouteForm component with calls to that helper (use
the helper name in each location to make intent clear and keep behavior
consistent across sibling routes).

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b5e7c8b8-b9ee-45ea-b19d-ce6edca840fb

📥 Commits

Reviewing files that changed from the base of the PR and between fb4dc25 and decb771.

📒 Files selected for processing (11)
  • src/components/CopyPageDropdown.tsx
  • src/components/DocsLayout.tsx
  • src/components/SearchModal.tsx
  • src/routes/$libraryId/$version.docs.tsx
  • src/routes/$libraryId/$version.index.tsx
  • src/routes/$libraryId/$version.tsx
  • src/routes/admin/audit.tsx
  • src/routes/admin/github-stats.tsx
  • src/routes/admin/logins.tsx
  • src/routes/admin/npm-stats.tsx
  • src/routes/admin/roles.$roleId.tsx

@tannerlinsley tannerlinsley merged commit c09fe03 into main Mar 17, 2026
8 checks passed
@tannerlinsley tannerlinsley deleted the site-reliability branch March 17, 2026 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants